-
-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce dependencies size #981
Conversation
cc: @ai I have tried to reduce the size of node_modules, but it didn't bring a lot: Update: earlier I mentioned that the size went up. That's not true: I've just installed the local package incorrectly. It turned out to be almost useless to try and remove such packages as Another dependency hoard comes from I could also go and patch What would be the best solution for this project? |
P. S. I can't quite understand why some tests are failing. Apparently, What I can understand is that |
Honestly, I think we are already using the most minimal amount of dependencies possible. Of course those dependencies might have a lot of extra cruft, but I don't think lint-staged directly has anything extra. I've tried to slim down unnecessary stuff and also keep them updated. Tests might fail because log symbols differ based on platform, and tests might work around this by mocking or serializing them differently. |
I also question this work unless there is something obvious. I think the easiest way to reduce the size is for all libraries to only ship a minimal required files but I don’t see how lint-staged itself can solve this. Also is 21 MB really a problem for a Dev dependency? To me this kind of work add little to no value but costs a lot of time. |
We could try a separate |
Hmm that’s interesting idea indeed. Is there a native npm behavior that could help us? Something like optional dependencies that are then installed on CI? I’m wondering why are dev dependencies are installed when ping-staged added as a dependency and if we could look into that somehow? |
Nope, the dev dependencies aren't being installed when performing |
20 MB of dependencies is a lot. The problem is that most of them is Seems like the How hard it will be to create a tiny analog or reimplement it in |
It is not is you will count sub-dependnecies. https://npm.anvaka.com/#/view/2d/lint-staged There are 94 dependencies in |
Maybe this effort could be focused directly on |
Yeap. Seems like it is better to send PR to If we accept this PR ( |
If |
One major feature to consider is the |
It is also to be noted, that Colorette, apparently, offers its own, albeit simpler implementation, where |
I have posted a PR to |
Nice work @NickKaramoff, seems like it got already merged. Maybe if we reduce this PR to only target replacing Let's handle the other dependencies in separate PR's. |
I have reverted the removal of I have also updated |
Now, when it comes to testing, I find it bizarre that the AppVeyor builds are now broken. Apparently Jest expects the CLI o=to output non-Unicode characters from Should we just update the Jest snapshots? |
Very strange that Unicode symbols were not used in CI. According to I found that in (I also think that we should not use |
I believe we use it might be that the Appveyor terminal doesn't suppor colors and so they're not used "on Windows"... what about the tests running on GitHub CI? |
The same. It is not connected with CI. If you run Jest locally |
|
Thanks for the clarification, it has been some time since I've touched these. This should be the auto mock file: I think it would be preferable to use the same packages as |
OK, I see. I'll figure out how to fix it and update the PR 👌
Theoretically yes, but it would save even more space if both |
@NickKaramoff it would perfectly fit us if we could import and use those same symbols directly from Something like |
Implemented this exactly in this way 🙌 I've tried my best to fix the CI. Some stuff (like extra white spaces) were existent before this PR (at least on my machine) and some new stuff was introduced, which has to do something with Git... I honestly do not understand this happens 😅 |
@NickKaramoff Honestly, I don't have any idea either. I've also had a lot of trouble trying to figure out why (especially) Appveyor tests behave like they do... It would be nice to ditch that and use GitHub Actions for all platforms. Now one thing that seems directly related in the test snapshots is this: The new line has a space at the end, while old doesn't: --- - ERROR
+++ + ERROR |
@iiroj sorry that it took to long to reply. This issue is weird, since this particular line of output you've linked to is being printed out by |
This PR #1003 merged an updated |
Replace 'cosmiconfig' with 'lilconfig' due to the smaller size and dependency tree of the latter package. This also installs 'js-yaml', since 'lilconfig' doesn't provide YAML parsing.
I have come back and rebased the changes made independently of this PR. Now it only replaces On a good note: I discovered the error with the tests. I didn't notice that I myself committed the wrong snapshots. This happened because of EditorConfig automatically removing trailing whitespace 😅 Now the tests should run smoothly |
Codecov Report
@@ Coverage Diff @@
## master #981 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 654 656 +2
Branches 149 149
=========================================
+ Hits 654 656 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this one. If there's other ways to reduce dependency size, we should open a separate PR for each individual change! 🚀
🎉 This PR is included in version 11.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 11.3.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR reduces the dependency tree of the package by replacing some dependencies with more lightweight alternatives. Closes #978.
Changes made
cosmiconfig
withlilconfig
js-yaml
sincelilconfig
doesn't provide YAML parsing